Skip to content

offload bucket per namespace - #4739

Closed
KannarFr wants to merge 11 commits into
apache:masterfrom
CleverCloud:tieredstorage-per-namespace
Closed

offload bucket per namespace#4739
KannarFr wants to merge 11 commits into
apache:masterfrom
CleverCloud:tieredstorage-per-namespace

Conversation

@KannarFr

Copy link
Copy Markdown
Contributor

I started to implement #4547.

@KannarFr
KannarFr force-pushed the tieredstorage-per-namespace branch from 1050eeb to b547171 Compare July 16, 2019 15:22

@sijie sijie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this PR only introduces the offload policies but it doesn't implement the logic to let the offloader using the offload polices. Is that done intentionally?

Comment thread pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Namespaces.java Outdated
Comment thread pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Namespaces.java Outdated
@KannarFr
KannarFr force-pushed the tieredstorage-per-namespace branch 2 times, most recently from c2f13c6 to 2696882 Compare July 17, 2019 11:52

@KannarFr KannarFr left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed OffloadType to String. And fixed other points.

@KannarFr

Copy link
Copy Markdown
Contributor Author

I added the logic about retrieving offloadPolicies for each NamespaceName, and get a Map<NamespaceName, LedgerOffloadManager>, there are todo on FileSystem and TieredStorage way to override default tiered storage configuration when a custom for NamespaceName exists. Now I need to implment the logic to schedule and run the offload.

@KannarFr

Copy link
Copy Markdown
Contributor Author

@sijie The goal is to work with your reviews between each implement choices to ensure I'm doing well.

@sijie
sijie requested a review from jiazhai July 18, 2019 16:46
@sijie sijie assigned sijie and KannarFr and unassigned sijie Jul 18, 2019
@sijie sijie added this to the 2.5.0 milestone Jul 18, 2019
@sijie

sijie commented Jul 18, 2019

Copy link
Copy Markdown
Member

@jiazhai can you please review this tiered storage related change?

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java Outdated
Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java Outdated
Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java Outdated
@jiazhai

jiazhai commented Jul 29, 2019

Copy link
Copy Markdown
Member

It would be good to add some test for this change.

@KannarFr
KannarFr force-pushed the tieredstorage-per-namespace branch from 258074f to 2e227e5 Compare July 30, 2019 08:45

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiazhai I removed it because of the new lazy load feature of offload policies even for non defined offloaders. WDYT about removing this?

@KannarFr
KannarFr force-pushed the tieredstorage-per-namespace branch from 2e227e5 to 7bbe81f Compare July 30, 2019 09:00
@KannarFr

KannarFr commented Aug 5, 2019

Copy link
Copy Markdown
Contributor Author

@sijie @jiazhai can you review before going further?

@sijie sijie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KannarFr overall looks good. However I didn't see how the offload policies used by actual offloader. do I miss anything?

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java Outdated
@KannarFr
KannarFr force-pushed the tieredstorage-per-namespace branch from 7bbe81f to 522c3de Compare August 12, 2019 12:17

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sijie If I'm correct, it's done here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then I don't know which ledgerFactory should be used, not sure about what their job :/ can you explain them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jiazhai maybe? :D

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cdbartholomew

Copy link
Copy Markdown
Contributor

I think this is a great feature. Is there anything I can do to help out?

@aahmed-se

Copy link
Copy Markdown
Contributor

@cdbartholomew rebase the pr on master and add more tests.

@sijie

sijie commented Nov 3, 2019

Copy link
Copy Markdown
Member

@aahmed-se I would suggest asking the original author first before taking over the PR.

@KannarFr

KannarFr commented Nov 3, 2019

Copy link
Copy Markdown
Contributor Author

Hey, was waiting for feedback about the way I already done some work. I will be pleased to have your help. Sure first thing is to rebase from master.

@cdbartholomew

Copy link
Copy Markdown
Contributor

Hey, was waiting for feedback about the way I already done some work. I will be pleased to have your help. Sure first thing is to rebase from master.

Ok, I'll rebase from master and take a look at adding some tests.

@sijie sijie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KannarFr the general direction of this change looks good to me.

@KannarFr

Copy link
Copy Markdown
Contributor Author

@cdbartholomew Hi, do you still want to work on it? :)

@cdbartholomew

Copy link
Copy Markdown
Contributor

@KannarFr sorry about taking so long to respond. I have been really busy and haven't been able to look at it. However, I still think this is a very useful change, and I think I have talked @zzzming into working on it. :-)

@KannarFr

KannarFr commented Jan 9, 2020

Copy link
Copy Markdown
Contributor Author

@cdbartholomew ok, I'm rebasing from master this PR.

@KannarFr
KannarFr force-pushed the tieredstorage-per-namespace branch from 522c3de to 06e4584 Compare January 9, 2020 16:23
@jiazhai

jiazhai commented Jan 18, 2020

Copy link
Copy Markdown
Member

retest this please

@gaoran10

Copy link
Copy Markdown
Contributor

@KannarFr Hi, this is a good feature, I want to use it in my project, do you want finish it ?

@sijie

sijie commented Jan 18, 2020

Copy link
Copy Markdown
Member

@gaoran10 I guess @KannarFr is not actively working on this at this moment. If there is a need, feel free to continue the work based on this pull request.

@gaoran10

Copy link
Copy Markdown
Contributor

@sijie @KannarFr I'm glad to complete the feature base on this pull request.

@KannarFr

Copy link
Copy Markdown
Contributor Author

@gaoran10 Hi, go for it :).

@KannarFr

Copy link
Copy Markdown
Contributor Author

@gaoran10 any news? :)

@gaoran10

Copy link
Copy Markdown
Contributor

@KannarFr Hi, I finish this feature at #6183, have a look please.

@KannarFr

Copy link
Copy Markdown
Contributor Author

@gaoran10 Oh, you rewrite all commits? Anyway, thanks for the feature, can't wait to use it :p.

@codelipenghui

Copy link
Copy Markdown
Contributor

@KannarFr The #6183 merged, I would close this PR.

@KannarFr
KannarFr deleted the tieredstorage-per-namespace branch February 10, 2020 12:57
@sijie

sijie commented Feb 13, 2020

Copy link
Copy Markdown
Member

Thanks @KannarFr for the contribution!

@codelipenghui

Copy link
Copy Markdown
Contributor

Since this feature is released in 2.5.1, so I added a release/2.5.1 tag for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants